Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): pass environment variables to CustomResourceProvider #10560

Merged
merged 2 commits into from
Oct 6, 2020
Merged

feat(core): pass environment variables to CustomResourceProvider #10560

merged 2 commits into from
Oct 6, 2020

Conversation

ayush987goyal
Copy link
Contributor

Add environment prop to CustomResourceProvider

closes #9668


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why one would need this. Wouldn't you pass all configuration into the CustomResource itself, not the provider?

packages/@aws-cdk/core/README.md Outdated Show resolved Hide resolved
@ayush987goyal
Copy link
Contributor Author

I don't understand why one would need this. Wouldn't you pass all configuration into the CustomResource itself, not the provider?

These configuration are needed for the lambda itself. I agree that same thing can potentially be achieved by passing the details to the CustomResource as well. But maybe the users would want to keep the properties of the received event to be lean and not dependent on other parameters that the lambda would need for any other reasons.

rix0rrr
rix0rrr previously requested changes Oct 5, 2020
return undefined;
}

const variables: { [key: string]: string } = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why is this not simply

return { Variables: env };

?

We're copying from a K/V map into a K/V map. Is there value in the copying?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done to ensure the hash of lambda asset does not change per deployment based on order of the keys (notice we are sorting the keys in line 198)

I got the reference for this from the Lambda Function implementation

packages/@aws-cdk/core/README.md Outdated Show resolved Hide resolved
@rix0rrr rix0rrr added the pr-linter/exempt-readme The PR linter will not require README changes label Oct 5, 2020
@ayush987goyal ayush987goyal requested a review from rix0rrr October 5, 2020 14:33
@mergify mergify bot dismissed rix0rrr’s stale review October 5, 2020 14:43

Pull request has been modified.

@rix0rrr rix0rrr changed the title feat(core): Add environment prop to CustomResourceProvider feat(core): pass environment variables to CustomResourceProvider Oct 6, 2020
@mergify
Copy link
Contributor

mergify bot commented Oct 6, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 3da2e68
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Oct 6, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 320ec72 into aws:master Oct 6, 2020
@ayush987goyal ayush987goyal deleted the pr/custom-res-env branch October 6, 2020 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CORE] Add "environment" prop to class CustomResourceProvider
3 participants